Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Emotion][microperf] Separate form max-width variable to its own utility #7948

Merged
merged 1 commit into from
Aug 7, 2024

Conversation

cee-chen
Copy link
Member

@cee-chen cee-chen commented Aug 6, 2024

Summary

As we've established in #7486, color computations are expensive to recalculate repeatedly and should be memoized where possible. This is fine for component styles (#7529), but our various euiComponentVariables utilities are not getting memoized except as part of component styles.

euiFormVariables is particularly chonky and likely bears examining more closely, but in the interim I'm solving this particular microperf irritant by creating a separate utility just to grab the maxWidth size for components that only need this token (a surprisingly high number).

My upcoming EuiComboBox conversion will be utilizing this PR as well for cleaner and more performant code ✨

QA

General checklist

  • Browser QA - N/A, fairly straightforward PR that doesn't need excessive cross-browser testing
  • Docs site QA - N/A
  • Code quality checklist - N/A, styles only
  • Release checklist - N/A, skipping a changelog as this is mostly tech debt/ an internal concern only
  • Designer checklist - N/A

…ed the single variable

- perf: color computations are expensive, and we don't need to calculate every single form var repeatedly for just one thing that's reused often
@cee-chen cee-chen marked this pull request as ready for review August 6, 2024 19:22
@cee-chen cee-chen requested a review from a team as a code owner August 6, 2024 19:22
@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

Copy link
Contributor

@mgadewoll mgadewoll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚢 🐈‍⬛ LGTM! If it became a larger performance issue across the board, we could consider separating colors vars and/or memoizing variables in general 🤔

Copy link
Member

@tkajtoch tkajtoch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes look great! 🚢

@cee-chen
Copy link
Member Author

cee-chen commented Aug 7, 2024

Thanks y'all!

@cee-chen cee-chen merged commit 005c64a into elastic:main Aug 7, 2024
13 checks passed
@cee-chen cee-chen deleted the emotion/form-max-width branch August 7, 2024 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants